[CHIA-3795] change v1 plot phase-out #20216
Conversation
arvidn
added
the
Changed
arvidn
force-pushed
the
plot-v1-phase-out
branch
2 times, most recently
from
75f8aa6 to
d1fb255
Compare
arvidn
force-pushed
the
plot-v1-phase-out
branch
from
d1fb255 to
a6ef930
Compare
|
|
||
| @pytest.fixture( | ||
| scope="session", | ||
| params=[ConsensusMode.PLAIN, ConsensusMode.HARD_FORK_2_0, ConsensusMode.HARD_FORK_3_0], |
There was a problem hiding this comment.
the test blockchains we use in tests were generated before the hard fork 2 and the phase out. These chains won't be valid when run as if hard fork 2 has activated. The proofs of space won't be valid. Once we have new chains we can enable these tests
| block.height, | ||
| diff, | ||
| ssi, | ||
| prev_tx_block(blocks, blocks.block_record(block.prev_header_hash)), |
There was a problem hiding this comment.
this is wrong you dont know that you have these blocks
There was a problem hiding this comment.
at this specific point we do know the previous transaction block height though. blocks is the Blockchain object, so we have all the blocks. It's in the other call we don't have the blockchain. Or am I misunderstanding this?
There was a problem hiding this comment.
oh, I see. This is BlockCache, and it may be empty if this is the first block in the loop. Do you think this should be fixe in this PR?
If so, do you suggest max(0, height - 50) as prev_tx_height?
There was a problem hiding this comment.
actually i was confusing this with __validate_pospace
here you can just skip until you see the first tx block and then start validation that should be fine
There was a problem hiding this comment.
so, unchanged then. right? your description is my understanding of what prev_tx_block() does.
There was a problem hiding this comment.
i think a better way to do it here is track the tx block as we loop over the recent chain, we are already counting tx blocks with transaction_blocks
since this is validation logic im not a fan of relying on whats underneath the BlockCache, althogh i guess it gets you the latest tx block from the current peak in the cache ?
There was a problem hiding this comment.
I'm not really an authority on this code. In order to change it I would need to become more familiar with it. I can't tell if you're asking for changes in this PR
There was a problem hiding this comment.
i can do it in another pr if you want
| ) -> Optional[bytes32]: | ||
| plot_size = pos.size() | ||
|
|
||
| if plot_size.size_v1 is not None and is_v1_phased_out(pos.proof, prev_transaction_block_height, constants): |
There was a problem hiding this comment.
i would probably add a log for this
| new_block_gen: Optional[NewBlockGenerator] | ||
| async with self.full_node.blockchain.priority_mutex.acquire(priority=BlockchainMutexPriority.high): | ||
| peak: Optional[BlockRecord] = self.full_node.blockchain.get_peak() | ||
| tx_peak: Optional[BlockRecord] = self.full_node.blockchain.get_tx_peak() |
There was a problem hiding this comment.
we are getting the last transaction block a few lines down (curr_l_tb) i think we should consolidate, its under the lock so should be the same but would be cleaner
There was a problem hiding this comment.
I saw that as well. I was hesitant to mix this PR with refactoring the code below. I think it would be simpler to just call get_tx_peak() and use that in both places.
| # validate proof of space | ||
| assert sub_slot_data.proof_of_space is not None | ||
|
|
||
| required_iters = validate_pospace_and_get_required_iters( |
There was a problem hiding this comment.
here we dont have information about where the previous transaction block is, passing 0 currently ignores the phaseout completely i think we can do better just give x blocks bellow height, wouldn't be accurate but i dont see other options
arvidn
force-pushed
the
plot-v1-phase-out
branch
from
3d769be to
78d1f92
Compare
|
to not affect required_iters which will change block infusion distribution in the slot and to be quantized to epoch blocksץ
The phase-out check is implemented in
is_v1_phased_out().The proof-of-space, when validated (as opposed to farmed) uses
verify_and_get_quality_string(), which now needs to take the previous transaction block height, to check the phase-out.When farming, we need to check the phase-out independently now. There are new calls to
is_v1_phased_out()inharvester_api.pyas well asblock_tools.py.calculate_iterations_quality()no longer needs to take sub slot iterators nor height, since it doesn't implement the phase-out anymore. The phase-out is now separate from computing quality.